Skip to content

doc: Clarify fs.access works on directories too.#7113

Closed
lance wants to merge 4 commits into
nodejs:masterfrom
lance:fs-access-doc-clarity
Closed

doc: Clarify fs.access works on directories too.#7113
lance wants to merge 4 commits into
nodejs:masterfrom
lance:fs-access-doc-clarity

Conversation

@lance

@lance lance commented Jun 2, 2016

Copy link
Copy Markdown
Member
Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc, fs

Description of change

Fixes: #7110

This is maybe more verbose than needed, since the same information is
repeated several times. An alternative, maybe a single short sentence at
the beginning is better. E.g.

Tests a user's permissions for the file or directory specified by
path. All modes work for either files or directories. mode is...

Fixes: #7110

This is maybe more verbose than needed, since the same information is
repeated several times. An alternative, maybe a single short sentence at
the beginning is better. E.g.

> Tests a user's permissions for the file or directory specified by
> path. All modes work for either files or directories. `mode` is...
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 2, 2016
@cjihrig

cjihrig commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

Could you just change the first sentence to:

Tests a user's permissions for the file or directory specified by path.

As you already did. Then, for F_OK and friends, just change "File" to path.

No need to be so verbose about file vs. directory. They are all just paths.
@lance

lance commented Jun 2, 2016

Copy link
Copy Markdown
Member Author

@cjihrig simplified... Just noticed that my editor trimmed some trailing whitespace. I assume this is not a problem.

Comment thread doc/api/fs.md Outdated
possible to create a mask consisting of the bitwise OR of two or more values.

- `fs.constants.F_OK` - File is visible to the calling process. This is useful
- `fs.constants.F_OK` - Path is visible to the calling process. This is useful

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant use path, formatted as code to reference the argument.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have realized... Fixed

@cjihrig

cjihrig commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

LGTM

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jun 2, 2016
Comment thread doc/api/fs.md Outdated
following constants define the possible values of `mode`. It is possible to
create a mask consisting of the bitwise OR of two or more values.
Tests a user's permissions for the file or directory specified by `path`.
`mode` is an optional integer that specifies the accessibility checks to be

@jasnell jasnell Jun 2, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/mode is/The mode argument is

@jasnell

jasnell commented Jun 2, 2016

Copy link
Copy Markdown
Member

Small nit, otherwise LGTM

@lance

lance commented Jun 6, 2016

Copy link
Copy Markdown
Member Author

@jasnell @cjihrig nits addressed

@cjihrig

cjihrig commented Jun 6, 2016

Copy link
Copy Markdown
Contributor

Still LGTM

jasnell pushed a commit that referenced this pull request Jun 6, 2016
This is maybe more verbose than needed, since the same information is
repeated several times. An alternative, maybe a single short sentence at
the beginning is better. E.g.

Fixes: #7110
PR-URL: #7113
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

jasnell commented Jun 6, 2016

Copy link
Copy Markdown
Member

Landed in d976d66

@evanlucas

Copy link
Copy Markdown
Contributor

This is not landing cleanly on the v6.x branch. @lance would you be interested in opening an additional PR targeting the v6.x branch?

@lance

lance commented Jun 16, 2016

Copy link
Copy Markdown
Member Author

@evanlucas will do. A question. It's not clear to me if @jasnell's dcccbfd#diff-acabf706a8aa070a8796e3573f7e4678 is intended to land in v6.x. If so, the docs should refer to fs.constants.R_OK etc. If not, fs.R_OK. Since there is this #6534 (comment), I'm going to assume that I should leave as fs.R_OK on v6.x branch for the time being, since that should work whether @jasnell's commit lands in the branch or not.

@lance lance mentioned this pull request Jun 16, 2016
3 tasks
@lance

lance commented Jun 16, 2016

Copy link
Copy Markdown
Member Author

@evanlucas @jasnell #7321

@evanlucas

Copy link
Copy Markdown
Contributor

@lance yea, the linked pr is semver-minor so won't be landing until the next semver-minor bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants